feat: prevent fswap and fbridge module in Submessages #123

da1suk8 · 2024-04-25T03:17:34Z

Description

This PR implements a filtering mechanism for specific message types within Submessages.
Specifically, messages related to fswap and fbridge are targeted by this filter, and any attempts to execute operations based on these message types will result in an error.

FYI: Finschia/finschia-sdk#1336, Finschia/finschia-sdk#1340

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Checklist:

I followed the contributing guidelines and code of conduct.
I have added a relevant changelog to CHANGELOG.md
I have added tests to cover my changes.
I have updated the documentation accordingly.
I have updated API documentation client/docs/swagger-ui/swagger.yaml

x/wasm/keeper/msg_dispatcher.go

Mdaiki0730 · 2024-04-25T04:59:03Z

Hello, I have two questions.

How do you filter when trying to send fswap or fbredge messages as Custom?
- ref: https://github.com/Finschia/wasmvm/blob/main/types/msg.go#L101
You are filtering within MessageDispatcher.DispatchSubmessages, but is it not necessary to consider filtering in cases where dispatch is performed from MessageDispatcher.DispatchMessage?
- ref: https://github.com/da1suk8/wasmd/blob/feat/filter-submessage/x/wasm/keeper/msg_dispatcher.go#L41-L51

170210 · 2024-04-25T05:44:34Z

@Mdaiki0730

How do you filter when trying to send fswap or fbredge messages as Custom?

From this pr (#118 ), custom querier has been disabled.

You are filtering within MessageDispatcher.DispatchSubmessages, but is it not necessary to consider filtering in cases where dispatch is performed from MessageDispatcher.DispatchMessage?

This pr looks like only filter fswap/fbridge in submessage not all messages. Is it correct?

Alternative queries for our original modules are going to be opened with the stargate query feature in later versions.

@da1suk8 But we haven't opened our original modules' Stargate Querier yet. If we remove this PR, can the tests still pass?

da1suk8 · 2024-04-25T07:56:43Z

@Mdaiki0730 @170210

This PR prevents the use of fbridge and fswap within submessages, not in all messages.
Thank you for your comments.

x/wasm/keeper/msg_dispatcher.go

da1suk8 · 2024-04-25T08:03:32Z

@170210

But we haven't opened our original modules' Stargate Querier yet. If we remove this PR, can the tests still pass?

Sorry I didn't quite understand, please explain it again.

Mdaiki0730 · 2024-04-25T08:48:20Z

@170210 @da1suk8

From this pr (#118 ), custom querier has been disabled.

thank you for your reply. The querier's Custom has been changed to not handle, but what I'm wondering about is the submessage's Custom.
querier: https://github.com/Finschia/wasmvm/blob/main/types/queries.go#L85
submessage: https://github.com/Finschia/wasmvm/blob/main/types/msg.go#L101

Mdaiki0730 · 2024-04-25T09:20:35Z

@da1suk8

This PR prevents the use of fbridge and fswap within submessages, not in all messages.

I'm worried about whether this is the right place to implement it.
Currently, you are filtering using MessageDispatcher.DispatchSubmessages, but since messenger.DispatchMsg is public, it will not be filtered if it is called without going through MessageDispatcher.DispatchSubmessages.
Please don't bother if the scope of this PR doesn't require us to consider this.

x/wasm/keeper/msg_dispatcher.go

da1suk8 · 2024-04-26T06:11:49Z

@Mdaiki0730

To clarify my understanding, in the context of a Submessage, MessageDispatcher.DispatchSubmessages is invoked, and have applied a filter there.

Does this mean that messenger.DispatchMsg is also called?
Or are you suggesting that it might be better to apply a filter within messenger.DispatchMsg itself?

Mdaiki0730 · 2024-04-26T06:59:45Z

Or are you suggesting that it might be better to apply a filter within messenger.DispatchMsg itself?

This is my opinion. But it doesn't seem to be a problem at the moment, so you can skip it.

zemyblue

wasmd/x/wasm/keeper/handler_plugin.go

Lines 84 to 107 in a348dc5

    
           func (h SDKMessageHandler) handleSdkMessage(ctx sdk.Context, contractAddr sdk.Address, msg sdk.Msg) (*sdk.Result, error) { 
        
           	if err := msg.ValidateBasic(); err != nil { 
        
           		return nil, err 
        
           	} 
        
           	// make sure this account can send it 
        
           	for _, acct := range msg.GetSigners() { 
        
           		if !acct.Equals(contractAddr) { 
        
           			return nil, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "contract doesn't have permission") 
        
           		} 
        
           	} 
        
           	// find the handler and execute it 
        
           	if handler := h.router.Handler(msg); handler != nil { 
        
           		// ADR 031 request type routing 
        
           		msgResult, err := handler(ctx, msg) 
        
           		return msgResult, err 
        
           	} 
        
           	// legacy sdk.Msg routing 
        
           	// Assuming that the app developer has migrated all their Msgs to 
        
           	// proto messages and has registered all `Msg services`, then this 
        
           	// path should never be called, because all those Msgs should be 
        
           	// registered within the `msgServiceRouter` already. 
        
           	return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "can't route message %+v", msg) 
        
           }

Is there any case where it's executed through here?

da1suk8 · 2024-05-02T14:31:39Z

@Mdaiki0730

How do you filter when trying to send fswap or fbredge messages as Custom?

ref: https://github.com/Finschia/wasmvm/blob/main/types/msg.go#L101

I will handle another PR
Thank you.

da1suk8 · 2024-05-02T14:39:02Z

@zemyblue

Is there any case where it's executed through here?

The relevant functions are executed in the order of 1, 2, 3, as shown below. Therefore, this PR is sufficient.
Thank you.

wasmd/x/wasm/keeper/msg_dispatcher.go

Line 104 in a348dc5

events, data, err = d.messenger.DispatchMsg(subCtx, contractAddr, ibcPort, msg.Msg)
wasmd/x/wasm/keeper/handler_plugin.go

Line 130 in a348dc5

events, data, err := h.DispatchMsg(ctx, contractAddr, contractIBCPortID, msg)

wasmd/x/wasm/keeper/handler_plugin.go

Lines 62 to 82 in a348dc5

    
           func (h SDKMessageHandler) DispatchMsg(ctx sdk.Context, contractAddr sdk.AccAddress, contractIBCPortID string, msg wasmvmtypes.CosmosMsg) (events []sdk.Event, data [][]byte, err error) { 
        
           	sdkMsgs, err := h.encoders.Encode(ctx, contractAddr, contractIBCPortID, msg) 
        
           	if err != nil { 
        
           		return nil, nil, err 
        
           	} 
        
           	for _, sdkMsg := range sdkMsgs { 
        
           		res, err := h.handleSdkMessage(ctx, contractAddr, sdkMsg) 
        
           		if err != nil { 
        
           			return nil, nil, err 
        
           		} 
        
           		// append data 
        
           		data = append(data, res.Data) 
        
           		// append events 
        
           		sdkEvents := make([]sdk.Event, len(res.Events)) 
        
           		for i := range res.Events { 
        
           			sdkEvents[i] = sdk.Event(res.Events[i]) 
        
           		} 
        
           		events = append(events, sdkEvents...) 
        
           	} 
        
           	return 
        
           }

zemyblue

This changes is not need, I added another PR to do same feature in finschia witouth this wasmd change. Finschia/finschia#352

da1suk8 · 2024-05-08T05:48:10Z

Closed via Finschia/finschia#352

da1suk8 added 2 commits April 25, 2024 12:11

feat: prevent fswap and fbridge module in Submessages

407a2fd

docs: add CHANGELOG

7a6420f

da1suk8 marked this pull request as ready for review April 25, 2024 03:36

da1suk8 requested review from loloicci, Kynea0b and 170210 as code owners April 25, 2024 03:36

da1suk8 requested a review from zemyblue April 25, 2024 03:36

loloicci reviewed Apr 25, 2024

View reviewed changes

x/wasm/keeper/msg_dispatcher.go Outdated Show resolved Hide resolved

x/wasm/keeper/msg_dispatcher.go Outdated Show resolved Hide resolved

fix: separate error handling for fswap and fbridge

1ce21a0

loloicci reviewed Apr 25, 2024

View reviewed changes

x/wasm/keeper/msg_dispatcher.go Outdated Show resolved Hide resolved

refactor: improve error message prepositions

58714d1

loloicci previously approved these changes Apr 25, 2024

View reviewed changes

zemyblue reviewed Apr 26, 2024

View reviewed changes

x/wasm/keeper/msg_dispatcher.go Outdated Show resolved Hide resolved

refactor: improve Stargate msg handling with denylist

2047fe7

da1suk8 dismissed loloicci’s stale review via 2047fe7 April 26, 2024 08:32

zemyblue reviewed Apr 26, 2024

View reviewed changes

da1suk8 requested a review from zemyblue May 2, 2024 14:39

zemyblue reviewed May 3, 2024

View reviewed changes

da1suk8 closed this May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: prevent fswap and fbridge module in Submessages #123

feat: prevent fswap and fbridge module in Submessages #123

da1suk8 commented Apr 25, 2024 •

edited

Loading

Mdaiki0730 commented Apr 25, 2024

170210 commented Apr 25, 2024 •

edited

Loading

da1suk8 commented Apr 25, 2024 •

edited

Loading

da1suk8 commented Apr 25, 2024

Mdaiki0730 commented Apr 25, 2024

Mdaiki0730 commented Apr 25, 2024 •

edited

Loading

da1suk8 commented Apr 26, 2024

Mdaiki0730 commented Apr 26, 2024 •

edited

Loading

zemyblue left a comment •

edited

Loading

da1suk8 commented May 2, 2024

da1suk8 commented May 2, 2024

zemyblue left a comment

da1suk8 commented May 8, 2024

	func (h SDKMessageHandler) handleSdkMessage(ctx sdk.Context, contractAddr sdk.Address, msg sdk.Msg) (*sdk.Result, error) {
	if err := msg.ValidateBasic(); err != nil {
	return nil, err
	}
	// make sure this account can send it
	for _, acct := range msg.GetSigners() {
	if !acct.Equals(contractAddr) {
	return nil, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "contract doesn't have permission")
	}
	}

	// find the handler and execute it
	if handler := h.router.Handler(msg); handler != nil {
	// ADR 031 request type routing
	msgResult, err := handler(ctx, msg)
	return msgResult, err
	}
	// legacy sdk.Msg routing
	// Assuming that the app developer has migrated all their Msgs to
	// proto messages and has registered all `Msg services`, then this
	// path should never be called, because all those Msgs should be
	// registered within the `msgServiceRouter` already.
	return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "can't route message %+v", msg)
	}

feat: prevent fswap and fbridge module in Submessages #123

feat: prevent fswap and fbridge module in Submessages #123

Conversation

da1suk8 commented Apr 25, 2024 • edited Loading

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Checklist:

Mdaiki0730 commented Apr 25, 2024

170210 commented Apr 25, 2024 • edited Loading

da1suk8 commented Apr 25, 2024 • edited Loading

da1suk8 commented Apr 25, 2024

Mdaiki0730 commented Apr 25, 2024

Mdaiki0730 commented Apr 25, 2024 • edited Loading

da1suk8 commented Apr 26, 2024

Mdaiki0730 commented Apr 26, 2024 • edited Loading

zemyblue left a comment • edited Loading

Choose a reason for hiding this comment

da1suk8 commented May 2, 2024

da1suk8 commented May 2, 2024

zemyblue left a comment

Choose a reason for hiding this comment

da1suk8 commented May 8, 2024

da1suk8 commented Apr 25, 2024 •

edited

Loading

170210 commented Apr 25, 2024 •

edited

Loading

da1suk8 commented Apr 25, 2024 •

edited

Loading

Mdaiki0730 commented Apr 25, 2024 •

edited

Loading

Mdaiki0730 commented Apr 26, 2024 •

edited

Loading

zemyblue left a comment •

edited

Loading